Skip to content

Always shadow for-loop control variables in macro bodies - #1158

Open
JJH090501 wants to merge 1 commit into
teal-language:mainfrom
JJH090501:macro-lua55
Open

Always shadow for-loop control variables in macro bodies#1158
JJH090501 wants to merge 1 commit into
teal-language:mainfrom
JJH090501:macro-lua55

Conversation

@JJH090501

@JJH090501 JJH090501 commented Aug 18, 2026

Copy link
Copy Markdown

Problem

A macro whose body writes to the control variable of a for loop fails to
compile when the compiler itself runs on Lua 5.5:

local macro shout!(x: Expression)
   local out = block('statements')
   for entry in ("a;b"):gmatch("[^;]+") do
      entry = entry .. "!"
   end
   table.insert(out, `$x`)
   return out
end

shout!(print('hi'))
$ lua5.5 ./tl run macro_bug.tl
[string "shout"]:3: attempt to assign to const variable 'entry'

Numeric for in a macro body fails the same way. The file works on 5.1, 5.3
and 5.4 — 5.5 is the first version to make the control variable a constant.

Fix

Macro bodies keep targeting the 5.1 subset, as discussed. Instead of detecting
whether a body writes to a control variable, macro_eval now inserts the
shadowing local into every for in the macro body before parsing. local x = x
is valid in every supported target and is a no-op when nothing writes to it, so
no detection is needed and the traversal from the previous revision is gone.

Both forin and fornum are covered. 39 lines in teal/macro_eval.tl; no
other source file is touched.

Tests

spec/lang/macro/for_control_var_spec.lua adds three cases: a numeric for
body that writes its control variable, a generic for body that writes its
control variable, and a loop that never writes one. Each asserts the expanded
output rather than just that parsing succeeded, so the number of iterations the
macro body actually performed is encoded in the result.

Two of the three fail on Lua 5.5 without this change. On 5.1–5.4 they pass
either way, since those versions allow the assignment — which is also why CI
does not catch this today (lua-version: ["5.4", "5.3", "5.2", "5.1", "luajit"]).

make selfbuild is clean and busted --suppress-pending spec/ passes.

Related

Numeric for has the same problem outside macros: lua_compat.adjust_code has
no fornum case, so ordinary Teal code that writes to a numeric loop variable
type checks and then emits invalid Lua on 5.5. That is the issue @bjornbm ran
into in #1159; it reproduces on master without this change and needs
lua_compat and visitors changes, so it is out of scope here. Happy to take
it separately if nobody else is on it.

@hishamhm

hishamhm commented Aug 18, 2026

Copy link
Copy Markdown
Member

Thank you for looking into this!

Note: I used AI Assistant for English Comment and some test cases.

Thanks for disclosing. I am not keen on reviewing large AI-generated PRs (and their very long PR descriptions 😅 ), but I'm happy to review contributor code and answer questions.

Would you prefer the forin shadow logic extracted into a leaf module (depending only on teal.ast) so lua_compat and macro_eval can share it instead of duplicating? Happy to redo it that way.

This experimental macro feature still has some rough edges in the design. One of them is the Lua version that the macro bodies use: for now we assumed that 5.1 would be a usable "lowest common denominator". But if we start using the running Lua version, we risk that we'll start to have Teal modules including macros that only work in one specific Lua version.

So I think we should keep targeting a single Lua version, effectively forcing a "Lua subset" for macro code, like we already do for the standard library availability inside macros. This would make macro code stable across target Lua VMs.

For this specific problem of the for variables, I think the easiest approach here that does not involve all that additional traversal would be to simply always patch in the shadow variable in all for loops in macro_eval. Given the orthogonal shape of blocks, this should be possible without encoding lots of assumptions about the shape of the tree (that is, all those rules in lines 211-255 and 268-281.

@JJH090501

Copy link
Copy Markdown
Author

Thanks for your detailed review! I was planning a backend framework for lua 5.3+ with Teal support.

I think I understand what you mean, and what i should do now. I'll simplify the implementation and keep the macro code compatible with the Lua 5.1 subset.

I'd appreciate one more chance to revise the PR based on your feedback. :)

@JJH090501 JJH090501 changed the title Generate macro bodies for the running Lua version Always shadow for-loop control variables in macro bodies Aug 19, 2026
@JJH090501

Copy link
Copy Markdown
Author

Pushed the rewrite. The macro body still targets the 5.1 subset; the shadow is
now inserted unconditionally in every for in the body, both forin and
fornum, so the detection traversal is gone. 37 lines in
teal/macro_eval.tl plus three test cases — nothing else touched.

(Force-pushed, so the history changed. :))

@Frityet

Frityet commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

Hmm, I could see this continue to cause issues in the future, it is pretty clear that a blanket just "target lua 5.1" really won't work. I think we need to just target whatever Lua version that is running the macro.

@hishamhm Thoughts?

@hishamhm

Copy link
Copy Markdown
Member

@Frityet I think the language the macros are written in should not vary. The idea is that a macro should work the same no matter which is your gen-target: the input language should be effectively a Lua subset (in terms of keywords, operators and sandboxed standard library subset).

If we can change the output language based on the running VM without disrupting the input language of macros (i.e. without surfacing Lua version differences for macro authors), then that would be fine.

I think what I mean is probably that we should start thinking of the macro language less as Lua and more like "untyped Teal".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants